Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let unsafe traits and autotraits be used as bound in dyn-safe trait #106604

Closed
wants to merge 1 commit into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 8, 2023

This PR implements the observation from #51443 (comment) that the where_clauses_object_safety future compatibility lint is unnecessarily restrictive in the case of where Self: Sync bounds for 2 reasons:

  1. As an unsafe trait, writing unsafe impl Sync for dyn Trait + '_ {} in order to make Trait's where Self: Sync methods callable on dyn Trait is something that the user would have brought upon themselves. That impl is wrong.

  2. As an autotrait, they can't even write such an impl in the first place.

Fixes dtolnay/async-trait#228.

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 8, 2023
@cjgillot
Copy link
Contributor

I tend to agree with the rationale for unsafe traits. For safe traits from foreign crates, the error may be a little surprising to the user. As the point of the lint is to remove an unsoundness, this PR has soundness implications, so I'd rather be prudent.
Marking for t-types discussion.

@cjgillot cjgillot added the I-types-nominated Nominated for discussion during a types team meeting. label Jan 11, 2023
@dtolnay
Copy link
Member Author

dtolnay commented Jan 11, 2023

@cjgillot
Copy link
Contributor

cc #50781 for bookkeeping.

@nbdd0121
Copy link
Contributor

I tend to agree with the rationale for unsafe traits. For safe traits from foreign crates, the error may be a little surprising to the user. As the point of the lint is to remove an unsoundness, this PR has soundness implications, so I'd rather be prudent. Marking for t-types discussion.

My opinion is the other way: I agree with this relaxation for auto traits, because they may be specified in dyn (e.g. dyn Trait + Sync) and can't be implemented by the user outside libcore. The unsafe trait case is less clear to me, as I feel that we are kind of overloading the unsafe keyword here: usually unsafe impl only means that you need to fulfill the contract of unsafe trait, but now there is an additional requirement imposed by the language.

@compiler-errors compiler-errors added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jan 18, 2023
@lcnr lcnr self-assigned this Jan 18, 2023
@lcnr
Copy link
Contributor

lcnr commented Jan 18, 2023

(touched on this in t-types triage https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2023-01-18.20meeting/near/322084489)

I strongly disagree with allowing for unsafe traits but not safe traits.

Adding the additional safety requirement "Implementations of this trait are forbidden from applying to trait objects while not applying to all Sized types implementing that trait" feels bad. We cannot sensibly document this requirement in a way that's discoverable by users. As said by @nbdd0121 the current meaning of unsafe impl is: this implementation satisfies the safety requirements specified for this trait. Extending this also cover a new default safety requirement is not worth the benefit you get by making methods with unsafe traits as bounds object safe.

For auto traits you're right that it is safe as auto traits cannot be implemented for trait objects. As described in #50781 (comment) that's not quite right for local auto traits, but I am definitely open to strengthen this requirement also also forbid it for local auto traits. That means that trait objects only implement an auto trait Foo if they have an explicit Foo bound in which case the underlying type is also forced to implement Foo, making this safe.

I am open to only making Self: AutoTrait bounds safe as long as the implementation of the orphan check is updated to also prevent this in the local crate (with a comment for why that restriction is needed). With that I would be open to merging this after a types team FCP.

@lcnr lcnr removed the I-types-nominated Nominated for discussion during a types team meeting. label Jan 18, 2023
@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2023
@dtolnay
Copy link
Member Author

dtolnay commented Jan 19, 2023

Thank you, I agree with the feedback and the proposed approach in #106604 (comment). I have opened a new PR #107082 implementing that approach.

@dtolnay dtolnay closed this Jan 19, 2023
@dtolnay dtolnay deleted the whereclause branch January 19, 2023 19:08
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 3, 2023
Autotrait bounds on dyn-safe trait methods

This PR is a successor to rust-lang#106604 implementing the approach encouraged by rust-lang#106604 (comment).

**I propose making it legal to use autotraits as trait bounds on the `Self` type of trait methods in a trait object.** rust-lang#51443 (comment) justifies why this use case is particularly important in the context of the async-trait crate.

```rust
#![feature(auto_traits)]
#![deny(where_clauses_object_safety)]

auto trait AutoTrait {}

trait MyTrait {
    fn f(&self) where Self: AutoTrait;
}

fn main() {
    let _: &dyn MyTrait;
}
```

Previously this would fail with:

```console
error: the trait `MyTrait` cannot be made into an object
 --> src/main.rs:7:8
  |
7 |     fn f(&self) where Self: AutoTrait;
  |        ^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#51443 <rust-lang#51443>
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
 --> src/main.rs:7:8
  |
6 | trait MyTrait {
  |       ------- this trait cannot be made into an object...
7 |     fn f(&self) where Self: AutoTrait;
  |        ^ ...because method `f` references the `Self` type in its `where` clause
  = help: consider moving `f` to another trait
```

In order for this to be sound without hitting rust-lang#50781, **I further propose that we disallow handwritten autotrait impls that apply to trait objects.** Both of the following were previously allowed (_on nightly_) and no longer allowed in my proposal:

```rust
auto trait AutoTrait {}

trait MyTrait {}
impl AutoTrait for dyn MyTrait {}  // NOT ALLOWED

impl<T: ?Sized> AutoTrait for T {}  // NOT ALLOWED
```

(`impl<T> AutoTrait for T {}` remains allowed.)

After this change, traits with a default impl are implemented for a trait object **if and only if** the autotrait is one of the trait object's trait bounds (or a supertrait of a bound). In other words `dyn Trait + AutoTrait` always implements AutoTrait while `dyn Trait` never implements AutoTrait.

Fixes dtolnay/async-trait#228.

r? `@lcnr`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 3, 2023
Autotrait bounds on dyn-safe trait methods

This PR is a successor to rust-lang#106604 implementing the approach encouraged by rust-lang#106604 (comment).

**I propose making it legal to use autotraits as trait bounds on the `Self` type of trait methods in a trait object.** rust-lang#51443 (comment) justifies why this use case is particularly important in the context of the async-trait crate.

```rust
#![feature(auto_traits)]
#![deny(where_clauses_object_safety)]

auto trait AutoTrait {}

trait MyTrait {
    fn f(&self) where Self: AutoTrait;
}

fn main() {
    let _: &dyn MyTrait;
}
```

Previously this would fail with:

```console
error: the trait `MyTrait` cannot be made into an object
 --> src/main.rs:7:8
  |
7 |     fn f(&self) where Self: AutoTrait;
  |        ^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#51443 <rust-lang#51443>
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
 --> src/main.rs:7:8
  |
6 | trait MyTrait {
  |       ------- this trait cannot be made into an object...
7 |     fn f(&self) where Self: AutoTrait;
  |        ^ ...because method `f` references the `Self` type in its `where` clause
  = help: consider moving `f` to another trait
```

In order for this to be sound without hitting rust-lang#50781, **I further propose that we disallow handwritten autotrait impls that apply to trait objects.** Both of the following were previously allowed (_on nightly_) and no longer allowed in my proposal:

```rust
auto trait AutoTrait {}

trait MyTrait {}
impl AutoTrait for dyn MyTrait {}  // NOT ALLOWED

impl<T: ?Sized> AutoTrait for T {}  // NOT ALLOWED
```

(`impl<T> AutoTrait for T {}` remains allowed.)

After this change, traits with a default impl are implemented for a trait object **if and only if** the autotrait is one of the trait object's trait bounds (or a supertrait of a bound). In other words `dyn Trait + AutoTrait` always implements AutoTrait while `dyn Trait` never implements AutoTrait.

Fixes dtolnay/async-trait#228.

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

warn: where_clauses_object_safety when adding default implementation for async_trait
6 participants